Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Conversion performance improvement #594

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversion performance improvement #594

wants to merge 8 commits into from

Conversation

Sofranac-Boro
Copy link

Summary

The purpose of this merge request is to find easy wins in improving the performance of converters.

Details and comments

I ran some tests to see where the bottleneck in conversion lies. I used the enlight11.mps file, and tried to convert it to QUBO. On my machine, this is the breakdown of timings in seconds per the conversion step for this instance:

reading in mps file:  0.011665105819702148
translating:  0.008737564086914062
<class 'qiskit_optimization.converters.linear_inequality_to_penalty.LinearInequalityToPenalty'>  converter:  0.014148950576782227
<class 'qiskit_optimization.converters.inequality_to_equality.InequalityToEquality'>  converter:  0.01540994644165039
<class 'qiskit_optimization.converters.integer_to_binary.IntegerToBinary'>  converter:  0.09988164901733398
<class 'qiskit_optimization.converters.linear_equality_to_penalty.LinearEqualityToPenalty'>  converter:  10.31824779510498
<class 'qiskit_optimization.converters.flip_problem_sense.MaximizeToMinimize'>  converter:  1.1920928955078125e-05
Total conversion time:  10.447956562042236

By far the most time is spent in the LinearEqualityToPenalty converter. Out of ~10.3 seconds spent in this converter, ~9.3 seconds are spent in the _coeffs_to_dok_matrix function while processing the quadratic coefficients of the resulting QUBO.

This function first builds the QUBO matrix out of a dictionary of coefficients in full form (both upper and lower triangular elements are added), and then transforms it into an upper diagonal form via matrix operations. As these operations can be expensive, the proposed change simply directly builds the matrix in upper triangular form, avoiding several intermediate steps

In the following table, the conversion time is compared before and after the improvements for a few instances:

Instance old new
enlight11.mps 10.6 6.6
mzzv11.mps 32.3 28.5
acc-tight5.mps 3.8 3.1
air04.mps 82.7 53.6
iis-glass-cov.mps 21.5 18.0
f2gap40400.mps 0.7 0.4
cod105.mps 13.3 11.0
     

✅ I have added the tests to cover my changes.
✅ I have updated the documentation accordingly. [nothing to be updated]
✅ I have read the CONTRIBUTING document.

@CLAassistant
Copy link

CLAassistant commented Feb 5, 2024 •
edited
Loading

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ woodsp-ibm
✅ Sofranac-Boro
❌ Boro Sofranac

Boro Sofranac seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Sofranac-Boro
Copy link
Author

Does anyone have any idea what could cause python3.8 to fail the QAOA test, while other python versions work? Maybe some of you had similar experiences before? Here's what I know:

  • tried running this test with python3.8 locally, it passes without problems.
  • All other python versions work fine. Also 3.8 passes on windows, but not on mac or ubuntu.
  • The offending test does indeed call the converter under the hood, which was touched by this MR. I double checked that the result of the conversion in the test is identical:
minimize 2*x0^2 + 0.5*x0*x1 + 0.5*x1^2 + 2*x0 + 2*x1 (2 variables, 1 constraints, 'op_ip1')
vs
minimize 2*x0^2 + 0.5*x0*x1 + 0.5*x1^2 + 2*x0 + 2*x1 (2 variables, 1 constraints, 'op_ip1')

The failure happens because the qaoa finishes with an suboptimal solution [1,1] (objective val 7) while cplex finds the optimum [0,2] (objective val 6)

@woodsp-ibm
Copy link
Member

woodsp-ibm commented Feb 7, 2024 •
edited
Loading

It does seem rather a puzzle as to why CI is failing here. Locally on a linux system I created a new env with Python 3..8.18, as its using here, and locally modifying the QuadraticExpression code the same the test works - whether I run it the same as done in CI, i.e using stestr, via make test or do it manually individually. So locally I was not able to reproduce the failure either. I can only assume that my env is not exactly the same, indeed Windows CI here uses a conda env so its not quite the same either and the libraries are a bit different for that platform too anyway.

One could try and put some temporary prints to come out in the log in an attempt to figure things out - but I can imagine this not being very productive. Locally I printed the coeffs built during the test that fails to those built the former way and looking over things they seemed the same - but the test passed for me anyways.

I could see 2 ways to proceed as-is with further trying to figure why it fails here - simply accept that for 3.8 it works, based on the fact that tests run outside locally work, and augment the test that is failing to skip it under Python 3.8, leaving whatever mystery is causing the failure be and that logic untested under 3.8 by CI. The other would be to change the code so it conditionally goes down the new, more performant, code path only for Python 3.9 or above, and for 3.8 therefore still does it as its currently done. That would still have it run and pass (as it does now hopefully!) in CI but a user a would not get the speed up under 3.8. Qiskit has been supporting Python versions until their EOL - for 3.8 thats 14th Oct 2024, so 8 months away where shortly thereafter I would imagine dropping 3.8 support anyway not long after Qiskit does so. I guess I like to see stuff tested so I would more lean to the 2nd option personally rather than assume it carries on working, based on having seen that at this moment it runs/passes locally. Either way still leaves the mystery unsolved but without being able to reproduce it locally it seems it may have to remain a mystery. And of course put some comment at the location of whatevers changed to workaround this about the CI failure and why things look like they do.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8298754055

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 6 of 10 (60.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.07%) to 92.959%
Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_optimization/problems/quadratic_expression.py 6 10 60.0%
Totals Coverage Status
Change from base Build 7890111189: -0.07%
Covered Lines: 4515
Relevant Lines: 4857
💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefan-woerner stefan-woerner Awaiting requested review from stefan-woerner stefan-woerner is a code owner

@woodsp-ibm woodsp-ibm Awaiting requested review from woodsp-ibm woodsp-ibm is a code owner

@t-imamichi t-imamichi Awaiting requested review from t-imamichi t-imamichi is a code owner

@dthuerck dthuerck Awaiting requested review from dthuerck

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Sofranac-Boro @CLAassistant @woodsp-ibm @coveralls